Skip to content

Support Custom-ID Markdown Syntax#1636

Merged
bvaughn merged 3 commits into
reactjs:masterfrom
smikitky:remark-autolink-custom-id
Feb 6, 2019
Merged

Support Custom-ID Markdown Syntax#1636
bvaughn merged 3 commits into
reactjs:masterfrom
smikitky:remark-autolink-custom-id

Conversation

@smikitky

@smikitky smikitky commented Feb 6, 2019

Copy link
Copy Markdown
Member

For the reason why this is necessary, see: #1608

This PR enables user-defined IDs on markdown headings, as explained in this guide.

A heading with an explicit custom ID would look like this:

## Try React {#try-react}

### What does const [thing, setThing] = useState() mean? {#destructuring}

To implement this, we introduce a new custom Gatsby plug-in, which will work in place of the official gatsby-remark-autolink-headers plugin (MIT Licensed).

  • Removed dependency on gatsby-remark-autolink-headers and copied most of its contents under plugins directory in this repository. The new custom plugin is called gatsby-remark-header-custom-ids.
  • Added a few lines of code to modify the AST. (e74a555)

To check how this works, edit some *.md file under content, and execute the following:

  1. yarn (removes the official plugin)
  2. yarn reset (this seems to be important after something changed in config)
  3. yarn dev or yarn build

Notes:

  • If no explicit custom ID is present, nothing changes; we just keep generating a slugified ID from the header text. So we can gradually adopt custom IDs on a per-page or per-heading basis.
  • Custom IDs must contain only lower-case alphabet, digits, hyphens and underscores.
  • This PR does not include a script to add explicit custom IDs to existing headings.

CC @tesseralis

@facebook-github-bot

Copy link
Copy Markdown
Collaborator

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

Copy link
Copy Markdown
Collaborator

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@reactjs-bot

Copy link
Copy Markdown

Deploy preview for reactjs ready!

Built with commit 16515ce

https://deploy-preview-1636--reactjs.netlify.com

@tesseralis tesseralis requested review from bvaughn and gaearon February 6, 2019 19:33

@bvaughn bvaughn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. In hindsight, it's a great idea.

I don't mind merging this to unblock our translations efforts– but given how close it is to gatsby-remark-header-custom-ids– maybe @KyleAMathews and the Gatsby community would like to see it merged upstream?

exports.shouldUpdateScroll = ({routerProps: {location}}) => {
const offset = getTargetOffset(location.hash);
return offset !== null ? [0, offset] : true;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a forked+formatted version of gatsby-remark-autolink-headers/src/gatsby-browser.js 👍

dangerouslySetInnerHTML={{__html: script}}
/>,
]);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a formed+formatted version of gatsby-remark-autolink-headers/src/gatsby-ssr.js 👍

// Remove the custom ID part from the text node.
const lastNode = node.children[node.children.length - 1];
lastNode.value = lastNode.value.replace(match[1], '');
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a formed+formatted version of gatsby-remark-autolink-headers/src/index.js with the exception of the above ~10 lines 👍

@tesseralis

Copy link
Copy Markdown
Contributor

@bvaughn we could always merge it in, and switch to the open-sourced plugin later!

@KyleAMathews

Copy link
Copy Markdown
Contributor

Would love this PRed upstream! We do merge and release pretty quickly so it might be easier to just PR your improvements to us.

@bvaughn

bvaughn commented Feb 6, 2019

Copy link
Copy Markdown
Contributor

😄 I've found it pretty easy to get contributes to Gatsby plug-ins merged quickly!

I'll merge this for now and we can revert once the upstream change has been published

@bvaughn bvaughn merged commit 98c1d22 into reactjs:master Feb 6, 2019
@smikitky

smikitky commented Feb 7, 2019

Copy link
Copy Markdown
Member Author

Thank you for merging. I believe this works in our use case, but right now this is not suitable for upstream because:

  • Probably, the more "correct" way to handle this is to extend the parser. The current regexp-based approach to patch AST is hacky.
  • Only lower-cased IDs are accepted, which is a requirement specific to this website.
  • I removed tests because sibling custom plugins didn't have ones, either :)

@bvaughn

bvaughn commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

I don't think I agree that the first two bullet points are blockers, and some basic tests could be written pretty easily 😄

@smikitky

smikitky commented Feb 7, 2019

Copy link
Copy Markdown
Member Author

Yes, I'll make a PR to the official plug-in tonight or this weekend :)

@bvaughn

bvaughn commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

That's awesome 😄 I think other Gatsby users will really appreciate the ability to customize header ids like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants